-
Notifications
You must be signed in to change notification settings - Fork 121
[POS as a tab i2] Update POS ineligible UI with detailed text and design updates #15859
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…Country`/`unsupportedCurrency` cases.
Generated by 🚫 Danger |
|
|
joshheald
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well, a few notes/suggestions inline 😊
| .foregroundColor(Color.posOnSurfaceVariantLowest) | ||
| } | ||
|
|
||
| VStack(spacing: 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we have POSSpacing.none if you want. It doesn't make a big difference though – I can't see us ever changing the value of that constant!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, updated in 33eb215.
| isLoading = false | ||
| } catch { | ||
| // TODO: WOOMOB-720 - handle error if needed, e.g., show an error message | ||
| print("Error refreshing eligibility: \(error)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: DDLogError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, updated in f95efc5.
| .buttonStyle(POSOutlinedButtonStyle(size: .normal)) | ||
| } | ||
| .containerRelativeFrame(.horizontal) { length, _ in | ||
| length * 0.5 - 132 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the width for the text and buttons to be at least 300px in 156eefe, given that the current iOS devices having 320px minimum width. There's still room for improvement for larger font sizes as you mentioned in WOOMOB-750.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| value: "Please update WooCommerce plugin to use POS.", | ||
| comment: "Ineligible reason: WooCommerce version too low") | ||
| return NSLocalizedString("pos.ineligible.suggestion.unsupportedIOSVersion", | ||
| value: "The POS system requires iOS 17 or later. Please update your device to iOS 17+ to use this feature.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| value: "The POS system requires iOS 17 or later. Please update your device to iOS 17+ to use this feature.", | |
| value: "Point of Sale requires iOS 17 or later. Please update your device to iOS 17+ to use this feature.", |
"The POS system" feels quite clumsy to read, I'd suggest replacing it with "Point of Sale" everywhere as it's clearer and shorter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy updated in 8ae5ba5.
| comment: "Ineligible reason: not a tablet") | ||
| return NSLocalizedString("pos.ineligible.suggestion.notTablet", | ||
| value: "Please use a tablet to access POS features.", | ||
| comment: "Suggestion for not tablet: use iPad") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the tab show up on the phone? I can't see it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not now and in the near future, but it's an eligibility check that I thought it's better to throw an ineligible error than fatalError or no-op. I can separate the ineligible reason enum into two, one for visibility and the other for eligibility so that this case won't have to be handled. Created an issue in WOOMOB-756.
| value: "POS feature is not enabled for your store.", | ||
| comment: "Ineligible reason: feature switch off") | ||
| return NSLocalizedString("pos.ineligible.suggestion.featureSwitchDisabled", | ||
| value: "The POS core feature must be enabled to proceed. " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| value: "The POS core feature must be enabled to proceed. " + | |
| value: "Point of Sale must be enabled to proceed. " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy updated in 8ae5ba5.
| comment: "Ineligible reason: feature switch off") | ||
| return NSLocalizedString("pos.ineligible.suggestion.featureSwitchDisabled", | ||
| value: "The POS core feature must be enabled to proceed. " + | ||
| "Please enable the POS feature from your WordPress admin under WooCommerce settings > Advanced > Features.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an API we can use to just do this when people tap a button on this screen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, there is pdfdoF-6VP-p2#endpoint, I created an issue to integrate with the API in WOOMOB-759.
| let countryNames = supportedCountries.map { $0.readableCountry } | ||
| let formattedCountryList = ListFormatter.localizedString(byJoining: countryNames) | ||
| let format = NSLocalizedString( | ||
| "pos.ineligible.suggestion.unsupportedCountry", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the tab in unsupported countries yet; I'm assuming that's just not changed yet 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the previous comment https://github.com/woocommerce/woocommerce-ios/pull/15859/files#r2182744134, it's because the ineligible enum includes the cases that make the POS tab invisible. I will handle that in the separate issue linked there.
| comment: "Suggestion for disabled feature flag: notify that POS is disabled remotely") | ||
| case .selfDeallocated: | ||
| return Localization.defaultReason | ||
| return NSLocalizedString("pos.ineligible.suggestion.selfDeallocated", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😬 hope we don't need this one!
|
Thanks for the testing and review feedback Josh! I believe I responded to all of your comments and am merging the PR now. Feel free to add more to the threads and I will respond to them after the merge. |





For WOOMOB-743
Just one review is required.
Description
Now that the design is available in y0zZLi1kqybNUUUd8lmvYG-fi-4133_4658, this PR updates the POS ineligible UI based on the design.
POS ineligible UI changes
WooCommerce/Classes/POS/TabBar/POSIneligibleView.swift: Updated thePOSIneligibleViewto show detailed suggestions instead of generic reasons for POS ineligibility. For example, unsupported countries now display a list of supported countries, and unsupported currencies show the supported currencies for the store settings. [1] [2]Eligibility updates
WooCommerce/Classes/ViewRelated/Dashboard/Settings/POS/POSTabEligibilityChecker.swift: Refactored thePOSIneligibleReasonenum to include additional context, such as supported countries and currencies. Updated the eligibility checker logic to use these new properties for more precise ineligibility handling. [1] [2]Test updates
WooCommerce/WooCommerceTests/ViewRelated/Settings/POS/POSTabEligibilityCheckerTests.swift: Updated existing test cases to validate the associated values for unsupported countries, currencies, and WooCommerce versions. [1] [2] [3] [4]Steps to reproduce
Prerequisite: logged in to a WPCOM account connected to a store not eligible for POS (e.g. WooCommerce version below 9.6.0-beta, unsupported currency) but in an eligible country (US/UK), and a store eligible for POS
POSTabEligibilityChecker.checkPluginEligibility) and how to fix itTesting information
Screenshots
Simulator.Screen.Recording.-.iPad.Air.13-inch.M3.-.2025-07-02.at.15.03.50.mp4
RELEASE-NOTES.txtif necessary.